fix(logging): 🐛 Update logging configuration to prevent empty log files and improve experiment log naming#137
Conversation
…ultAnalyzer This commit adds a check in the `ResultAnalyzer` class to raise an `AttributeError` if data has not been loaded before accessing it. This enhancement ensures that users are informed to call `read_data()` first, improving the robustness of the analysis utilities.
…es and improve experiment log naming This commit modifies the logging configuration in `absespy.yaml` to disable Hydra's default file handler, preventing the creation of empty log files. Additionally, it enhances the experiment logging setup in `exp_logging.py` to use the experiment name for log files when not explicitly set, ensuring clearer log file identification. Updates to the `fire_spread` example configuration provide a comprehensive logging setup for both experiment and model run levels. Tests are also updated to reflect these changes.
📝 WalkthroughWalkthroughRefactors logging: disables Hydra's default file handler, derives experiment log filenames from exp.name, propagates model logger to the root logger so user-module logs go to model files, adds a data-access guard in ResultAnalyzer, expands example logging config, and updates tests/type hints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
abses/utils/exp_logging.py (1)
147-177: Consider extracting helper functions to reduce duplication.The
exp_nameextraction logic (lines 150-156) andoutpathextraction logic (lines 160-175) are duplicated from the earlier branch (lines 108-114 and 120-134). While the current implementation is correct, extracting these into small helper functions would improve maintainability.🔎 Example helper extraction
def _get_exp_name(cfg: DictConfig | dict) -> str | None: """Extract exp.name from config.""" if isinstance(cfg, dict): return cfg.get("exp", {}).get("name") try: return OmegaConf.select(cfg, "exp.name", default=None) except Exception: return None def _get_outpath(cfg: DictConfig | dict) -> Path: """Extract and normalize outpath from config.""" if isinstance(cfg, dict): outpath = cfg.get("outpath") else: try: outpath = OmegaConf.select(cfg, "outpath", default=None) except Exception: outpath = None if outpath is None: return Path.cwd() elif isinstance(outpath, str): return Path(outpath) elif not isinstance(outpath, Path): return Path(str(outpath)) return outpath
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
abses/conf/absespy.yamlabses/utils/analysis.pyabses/utils/exp_logging.pydocs/api/analysis.mdexamples/fire_spread/config.yamltests/utils/test_logging.py
🧰 Additional context used
🧬 Code graph analysis (2)
abses/utils/exp_logging.py (2)
abses/core/experiment.py (2)
cfg(182-184)cfg(187-200)abses/utils/analysis.py (2)
select(117-129)select(329-338)
tests/utils/test_logging.py (3)
abses/core/experiment.py (2)
cfg(182-184)cfg(187-200)abses/utils/log_parser.py (3)
get_log_mode(30-84)get_stdout_config(159-184)get_mesa_config(220-240)abses/utils/log_config.py (2)
create_console_handler(79-100)create_file_handler(103-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (8)
docs/api/analysis.md (1)
11-11: LGTM!Minor formatting adjustment adding trailing newline. No functional impact.
examples/fire_spread/config.yaml (1)
43-77: Well-structured example configuration.The new multi-section logging configuration clearly demonstrates the hierarchical logging setup with separate
exp,run, andmesasections. The use ofnullvalues for optional fields likerotation,retention, and mesa overrides appropriately signals inheritance behavior.abses/utils/exp_logging.py (1)
106-117: Approve the improved log file naming logic.The logic correctly derives the experiment log file name from
exp.namewhen not explicitly set, which aligns with the PR objective of improving experiment log naming.abses/utils/analysis.py (1)
191-195: LGTM!Good defensive guard that provides a clear error message. While
read_data()is called during__init__, this guard protects against edge cases where_datamight not be set (e.g., ifread_data()is overridden or if_datais deleted).tests/utils/test_logging.py (3)
52-52: LGTM on type hint improvements.Consistent addition of
-> Nonereturn types andPathtype annotations improves code clarity and enables better static analysis.Also applies to: 57-57, 65-65, 85-85, 91-91, 114-114, 123-123, 130-130, 141-141, 147-147, 157-157, 164-164, 175-175, 189-189, 201-201, 213-213, 222-222, 237-237, 255-255, 283-283, 325-325, 339-339, 354-354, 390-390, 395-395, 446-446, 484-484, 516-516
130-135: Correct assertion update for mesa config defaults.The test now correctly asserts
Noneforlevelandformat, which aligns withget_mesa_configreturningNonevalues when mesa config is not explicitly set (allowing inheritance from run.file settings).
304-319: Good test for the new exp.name-based log file naming.This test validates that in separate mode, when
exp.nameis set, the experiment log file uses that name (test_experiment.log) rather than a mode-derived name.Consider adding a complementary test case for separate mode when
exp.nameis not set, to verify the fallback to"experiment"as the default log file name:def test_setup_exp_logger_separate_mode_no_exp_name(self, tmp_path: Path) -> None: """Test separate mode fallback when exp.name is not set.""" cfg = OmegaConf.create( { "outpath": str(tmp_path), # No exp.name set "log": { "mode": "separate", "exp": {"file": {"enabled": True}}, }, } ) _ = setup_exp_logger(cfg) # Should fallback to "experiment.log" assert (tmp_path / "experiment.log").exists()abses/conf/absespy.yaml (1)
27-29: Remove thefile: nullentry—it does not properly disable Hydra's file handler.Setting
file: nullunder handlers is not a recognized or supported Hydra pattern. The web search confirms that Hydra does not reliably process null-valued keys for removing handlers. The default configuration already lacks a file handler (only console handler is defined), so the addedfile: nullline is unnecessary and incorrect.To properly disable file logging in Hydra 1.3.2, use one of these approaches:
- Replace the job_logging config entirely with a custom version that omits the file handler
- Use the built-in preset:
hydra/job_logging=noneorhydra/job_logging=disabledSince the default config already has no file handler, no change is needed.
Likely an incorrect or invalid review comment.
This commit improves the logging configuration by ensuring that user module logs are captured in the model log files. It updates the `absespy.yaml` to clarify the role of the root logger and modifies the `Experiment` class to provide an experiment-level logger. Additionally, the `setup_model_logger` function is enhanced to configure the root logger appropriately. Changes in the `fire_spread` example demonstrate the new logging capabilities, and tests are added to verify that user module logs are correctly written to the model log file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @examples/fire_spread/model.py:
- Line 182: The log call exp.logger.info(f"Experiment {exp.name} started") is
placed after exp.batch_run(), which is misleading; either move this
exp.logger.info call to immediately before exp.batch_run() to correctly announce
start, or if you intend to log completion leave it after batch_run() but change
the message to something like "Experiment {exp.name} completed" (or similar) so
the message matches the actual execution point.
🧹 Nitpick comments (1)
abses/utils/logging.py (1)
181-200: LGTM! Root logger propagation properly implemented.The root logger configuration ensures user module logs are captured alongside ABSESpy/Mesa logs. The implementation correctly:
- Preserves Hydra handlers
- Prevents handler duplication
- Shares handlers to avoid duplicate file writes
Minor note: The comment on line 197 mentions "Create a copy" but the code actually shares the handler reference, which is correct for FileHandlers (prevents duplicate writes). Consider clarifying the comment.
🔎 Optional: Clarify the handler sharing comment
- # Add the same handlers from abses_logger to root logger for handler in abses_logger.handlers: - # Create a copy of the handler to avoid sharing state - # For FileHandler, we can share the same file + # Share the same handler to avoid duplicate writes to the same file if handler not in root_logger.handlers: root_logger.addHandler(handler)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
abses/conf/absespy.yamlabses/core/experiment.pyabses/core/model.pyabses/utils/logging.pyexamples/fire_spread/config.yamlexamples/fire_spread/model.pytests/utils/test_logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- abses/conf/absespy.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
examples/fire_spread/model.py (2)
abses/core/experiment.py (2)
logger(195-211)name(183-192)abses/utils/log_config.py (2)
debug(471-473)info(475-477)
tests/utils/test_logging.py (4)
abses/utils/log_parser.py (1)
get_log_mode(30-84)abses/utils/log_config.py (2)
create_console_handler(79-100)create_file_handler(103-162)abses/utils/logging.py (1)
setup_model_logger(102-202)abses/core/model.py (2)
name(158-165)outpath(168-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (12)
abses/core/model.py (1)
146-149: LGTM! Logging initialization moved to capture user logs during initialization.Moving the logging setup before
initialize()ensures that any user code logging within the initialization phase is properly captured. This aligns well with the root logger propagation introduced inabses/utils/logging.py.examples/fire_spread/model.py (3)
18-18: LGTM! Standard module-level logger setup.The module-level logger follows Python logging best practices and will be properly captured by the root logger propagation configured in
abses/utils/logging.py.Also applies to: 27-28
76-76: LGTM! Appropriate debug logging for tree ignition.The debug-level log provides useful runtime diagnostics with position information, which is helpful for troubleshooting fire spread behavior.
123-123: LGTM! Clear initialization milestone logging.The info-level log provides useful feedback about the initialization process and confirms the number of trees grown.
examples/fire_spread/config.yaml (1)
42-77: LGTM! Well-structured hierarchical logging configuration.The new logging configuration is comprehensive and clearly documented. The hierarchical structure (exp/run/mesa) with separate stdout and file settings provides good flexibility.
Minor observation:
run.stdout.enabled: false(line 62) means model run logs won't appear on stdout in separate mode - verify this is intentional for the example.abses/core/experiment.py (3)
163-175: LGTM! Clean experiment logger initialization.The logger initialization properly handles both
DictConfiganddicttypes, creates copies to avoid mutation, and converts the outpath to a string for serialization. The lazy initialization pattern is appropriate.
182-192: LGTM! Clean property for experiment name.The
nameproperty provides a clean interface to retrieve the experiment name from configuration with a sensible default. The type handling for bothdictandDictConfigis appropriate.
194-211: LGTM! Well-documented experiment logger property.The
loggerproperty provides a clean public API for experiment-level logging with appropriate lazy initialization and fallback. The docstring clearly explains the purpose and provides usage examples.tests/utils/test_logging.py (4)
52-52: Excellent type annotation improvements!The addition of explicit return type annotations (
-> None) and parameter type hints (Path) across all test methods improves type safety, IDE support, and code documentation.Also applies to: 57-57, 65-65, 85-85, 91-91, 114-114, 123-123, 130-130, 141-141, 147-147, 157-157, 164-164, 175-175, 189-189, 201-201, 213-213, 222-222, 237-237, 255-255, 283-283, 304-304, 325-325, 339-339, 354-354, 390-390, 395-395, 446-446, 484-484, 516-516
134-135: Correct behavioral assertion update.The test now correctly expects
Nonefor bothlevelandformatwhen Mesa configuration is not provided, aligning with the updated implementation behavior.
309-309: Correctly tests new exp.name-based log file naming.The test properly validates the new behavior where experiment log filenames are derived from
exp.name(set to "test_experiment" on Line 309) when not explicitly configured, resulting in "test_experiment.log" as expected on Line 319.Also applies to: 318-319
579-617: Excellent test coverage for user module log propagation!This new test comprehensively validates the critical feature where user module loggers (e.g.,
logging.getLogger(__name__)) now propagate to the model log file via root logger configuration. The test:
- Correctly sets up the model logger with file-only logging
- Simulates a user module logger
- Verifies the message appears in the model log file with the correct logger name
- Properly cleans up all handlers, including the root logger
| """ | ||
| exp = Experiment(Forest, cfg=cfg) | ||
| exp.batch_run() | ||
| exp.logger.info(f"Experiment {exp.name} started") |
There was a problem hiding this comment.
Fix the log message placement or wording.
The log message says "Experiment {exp.name} started" but it's placed after exp.batch_run() completes. This is misleading and doesn't reflect the actual execution flow.
🔎 Suggested fix
Option 1: Move the log before batch_run
exp = Experiment(Forest, cfg=cfg)
+exp.logger.info(f"Experiment {exp.name} started")
exp.batch_run()
-exp.logger.info(f"Experiment {exp.name} started")Option 2: Change the message to reflect completion
exp = Experiment(Forest, cfg=cfg)
exp.batch_run()
-exp.logger.info(f"Experiment {exp.name} started")
+exp.logger.info(f"Experiment {exp.name} completed")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @examples/fire_spread/model.py at line 182, The log call
exp.logger.info(f"Experiment {exp.name} started") is placed after
exp.batch_run(), which is misleading; either move this exp.logger.info call to
immediately before exp.batch_run() to correctly announce start, or if you intend
to log completion leave it after batch_run() but change the message to something
like "Experiment {exp.name} completed" (or similar) so the message matches the
actual execution point.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.